Remove quinn-proto rustls config newtypes #511
Conversation
Ralith
force-pushed
the
unwrap-rustls-config
branch
2 times, most recently
from
d604523 to
896e653
Compare
| @@ -87,6 +87,11 @@ pub trait ClientConfig<S> | |||
| where | |||
| S: Session, | |||
| { | |||
| /// Construct the default configuration | |||
There was a problem hiding this comment.
Should we use Default for this, instead?
There was a problem hiding this comment.
We can't implement Default on Arc<rustls::Client/ServerConfig>.
| } | ||
| } | ||
|
|
||
| /// A single TLS certificate |
There was a problem hiding this comment.
With the newtypes removed from quinn-proto, they're only used in the quinn builders, so they seemed out of place here.
There was a problem hiding this comment.
I feel like they should live closer to the rustls implementation bits, so that they could also be swapped out along with them. Do you think that would make sense?
There was a problem hiding this comment.
They also seem like things that a quinn-proto user might still find useful if they wanted to bypass the tokio stuff.
There was a problem hiding this comment.
I feel like they should live closer to the rustls implementation bits, so that they could also be swapped out along with them.
Other cryptographic protocols (e.g. noise) won't necessarily have analogous abstractions. For the specific case of other TLS implementations, we might prefer to expose a single abstract type that can be consumed by all backends, though that's pretty speculative.
They also seem like things that a quinn-proto user might still find useful if they wanted to bypass the tokio stuff.
The sole function of these types was to allow certificates/keys to be constructed without leaving the crate and passed into the helper methods that were removed, and the analogous builder methods in quinn which have been retained. While there's nonzero value in providing those helpers to hypothetical quinn-proto users, I think the resulting quinn-proto API was gratuitously weird for both quinn and quinn-proto users who need to access the rustls configuration directly, and maintaining two separate incomplete abstractions over rustls configurations wasn't a great situation.
Meanwhile, the genuine rustls API is perfectly usable. The justification for wrapping it as all is to maximize approachability for a new high-level API user, but at the quinn-proto level I think the calculus is different.
These were not successful at removing rustls from our public API, and made specifying custom cryptographic configurations unintuitive. Removing them reduces the scope of our public API while preserving the friendliness of the high-level quinn API.
Ralith
force-pushed
the
unwrap-rustls-config
branch
from
896e653 to
a371dd6
Compare
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
This is the second time we flip-flop on this. Previously in 4fd4868 (#351) we moved these types from quinn into quinn-proto so that they could be used as part of the low-level configuration API. Then in 9522b44 (#511) we moved them back into quinn after some discussion, making the point that the configuration API at the quinn-proto was not in a great place. Here, we keep the existing quinn-proto configuration API and merely offer an abstracted API relying on the wrapper types for use in quinn internals.
These were not successful at removing rustls from our public API, and made specifying custom cryptographic configurations unintuitive. Removing them reduces the scope of our public API while preserving the friendliness of the high-level quinn API.